Remove duplicate procedures from `miden-lib` #1002

Fumuran · 2024-12-03T00:19:14Z

This PR moves the duplicate masm procedures to the new utils library.

Todo: move the is_id_eq procedure to the utils module after #982 is merged.

Closes: #836

Fumuran · 2024-12-05T16:08:24Z

This solution implements the whole new utils library. Not sure that this is the best approach, it may be that creation of the just one utils.masm file will be better.

bobbinth

Thank you! Looks good. I left a few comment inline, but basically, there are two properties I'd like to retain:

Ideally, we won't need to add a new library to the list of libraries that we need to load into the executor and transaction kernel assembler.
Also, we should try to avoid changing the public interface of the modules in miden-lib (would should be able to just re-export procedures from the modules in which they were previously defined).

We could have achieved both of these if we had vendored library support in the assembler, but without it, I think we may need to rely on adding the MASM file with common utilities to tx kernel and miden lib libraries manually at build time.

miden-lib/asm/miden/note.masm

bobbinth · 2024-12-06T06:09:54Z

miden-lib/asm/miden/account.masm

-proc.type
-    u32split swap drop push.ACCOUNT_TYPE_U32MASK u32and
-    # => [acct_type]
-end


Same comment as above.

bobbinth · 2024-12-06T06:14:35Z

miden-lib/src/utils_lib.rs

+#[derive(Clone)]
+pub struct UtilsLib(Library);


Let's add a brief doc-comment explaining what this library contain.

bobbinth · 2024-12-06T06:20:36Z

miden-tx/src/executor/mast_store.rs

+        // load utils lib MAST forest
+        let utils_lib_forest = UtilsLib::default().mast_forest().clone();
+        store.insert(utils_lib_forest);


I'm not sure I like that we now need to care about another library. Ideally, we'd just vendor the utils library into the miden library at compile time, but unfortunately, Miden assembler does not yet support vendoring. So, maybe trying the approach with a single MASM file is worth it.

cc @plafer, @bitwalker for another use case for vendored library.

…d kernel libs

bobbinth · 2024-12-27T00:20:04Z

Overall, I think this looks good, but as discussed offline, I wonder if we should take this as an opportunity to refactor things a bit more in the direction mention in #982 (comment). Basically, try to identify pure "utility" procedures and move them out into separate "util" modules.

bobbinth · 2024-12-27T00:21:39Z

miden-lib/asm/utils.masm

+#! - acct_id_{hi,lo} are the first and second felt of an account id.
+#! - other_acct_id_{hi,lo} are the first and second felt of the other account id to compare against.
+#! - is_id_equal is a boolean indicating whether the account ids are equal.
+export.is_id_eq


This procedure (and procedures above) could maybe go into account_id module or something similar (e.g., account_id::are_equal.

I'm not sure I'm following. Do you mean to split utils into several modules like account, asset and note, as it was when it was a library? And I'm not sure that account_id as a name for one of these modules will fit, is_id_eq is the only procedure which is working with account IDs.

Yes, I'm thinking of creating several modules that separate "stateful" and "stateless" procedures. I haven't looked through everything to know where it'd make sense, but for account ID there are at least several "stateless" procedures that can go into an account_id module - e.g., is_id, is_fungible_faucet, is_non_fungible_faucet, is_faucet, is_updatable_account, is_immutable_account, validate_id.

Ah, I see, so you meant not just splitting the existing utils module, but combining it with the "pure utils" modules created from the corresponding kernel modules. Got it!

PhilippGackstatter · 2025-01-14T16:12:31Z

I think this might be ready now. I moved all pure procedures to one of the utils modules and fixed the tests. The latter required adding the utils modules to the TransactionKernel::testing_assembler. Not sure if there's a better solution there.

Fumuran

@PhilippGackstatter Looks great!
The only think I'm wondering about is whether we need to reexport the utils procedures from the main files (like account_utils procs from the account) to leave the public interface unchanged, as Bobbin said here.

Honestly, I forgot about it after we decide to make a bigger refactoring, introducing the new *_utils modules, since it looked as we need to change public interface anyway. But the issue with testing assembler reminded me about it (I had it previously): if we reexport utils procedures in the main modules, we don't need to add the utils module manually to the testing assembler — its mast forest will be in the kernel library.

I'm not sure what's better: on the one hand, if we reexport all these procedures user won't need to think where to get the desired procedure — just call it from corresponding module. On the other hand, currently we have quite a lot procedures in kernel modules, so it could be better to separate utils from non-utils to make it easier and slightly less confusing to operate them. But if we decide to reexport all of them, it looks like we are loosing the initial idea of moving utils procedures to another module.

@bobbinth what do you think?

Fumuran · 2025-01-14T20:47:51Z

miden-lib/src/transaction/mod.rs

+        #[cfg(feature = "std")]
+        let mut assembler = assembler;
+        // We don't build the utils modules as a library in build.rs so we have to load it here
+        // instead. This means it won't be included in no_std environments.


Is it more efficient to create a new mutable value inside the feature block than just make the original one mutable?
Or, in other words, will it be less efficient just to make the assembler instance at line 362 mutable and remove these lines (370-373) moving the comment inside the feature block?

bobbinth

Thank you! Looks good. I left a few comments inline, the main one being about trying avoid the manipulations of the testing assembler.

There is also a slight different approach where we have common modules which we add into different libraries under different namespaces. A good example of this is the assets module. If we have a module that is a combination of the current asset_utils module and miden::assets module, we could just add it to both libraries under different namespaces (miden:assets and kernel::assets).

We could do the same with account_utils. I think most of the procedures there are actually about account ID. So, maybe we create a common module named account_id and add it to the kernel and Miden library as kernel::account_id and miden::account_id respectively.

bobbinth · 2025-01-15T02:03:45Z

miden-lib/asm/utils/account_utils.masm

+# HELPER PROCEDURES
+# =================================================================================================


nit: seems like all procedures starting on line 273 are helper procedures. Should we move the separator there?

miden-lib/asm/kernels/transaction/lib/asset.masm

miden-lib/asm/kernels/transaction/lib/asset_vault.masm

miden-lib/asm/utils/asset_vault_utils.masm

bobbinth · 2025-01-15T06:49:50Z

miden-lib/build.rs

+    let utils_namespace = LibraryNamespace::new("utils").expect("invalid namespace");
+    let utils_path = Path::new(ASM_DIR).join(UTILS_DIR);


One other way to do this is to set the namespace here to be kernel::utils to make it specific to the kernel. We could also do the same for the miden library but importing the same files under miden::utils namespace.

bobbinth · 2025-01-15T06:50:42Z

miden-lib/src/transaction/mod.rs

+    /// This assembler also includes the `utils` namespace which contains the modules defined
+    /// in the utils directory (only under feature `std`).


I wonder if we do what I suggested in the last comment, would we be able to avoid needing to manually import utils here?

bobbinth · 2025-01-15T07:13:25Z

miden-lib/asm/utils/asset_utils.masm

@@ -0,0 +1,193 @@
+use.utils::account_utils


This import may cause an issue with my suggestions form the last comments. But I wonder if we can get rid of it. There are 3 procedures in this module which rely on account_utils and as far as I can tell, these procedures are not "shared". So, if we move them back into the kernel asset module, we could get rid of this import.

PhilippGackstatter · 2025-01-15T09:52:37Z

There is also a slight different approach where we have common modules which we add into different libraries under different namespaces. A good example of this is the assets module. If we have a module that is a combination of the current asset_utils module and miden::assets module, we could just add it to both libraries under different namespaces (miden:assets and kernel::assets).

I really like this idea. Not sure if you deliberately wrote kernel::assets instead of kernel::asset to avoid the name collision with the existing asset module, but if we could make a single kernel::asset module with all of the procedures, that would be best. Otherwise it's confusing to have a kernel::asset and kernel::assets. How would users know from which to import?

But the assembler doesn't allow defining a single kernel::asset from multiple "inputs", it results in duplicate definition found for module 'kernel::asset'. For account_id this works nicely as there is no collision, but for asset and note we'd have to choose different names and then I'm not sure how much benefit there is then.
So maybe re-exporting is the best strategy for asset and note?

bobbinth · 2025-01-15T09:57:19Z

Not sure if you deliberately wrote kernel::assets instead of kernel::asset to avoid the name collision with the existing asset module, but if we could make a single kernel::asset module with all of the procedures, that would be best. Otherwise it's confusing to have a kernel::asset and kernel::assets. How would users know from which to import?

Yes, sorry - it should be one module.

But the assembler doesn't allow defining a single kernel::asset from multiple "inputs", it results in duplicate definition found for module 'kernel::asset'. For account_id this works nicely as there is no collision, but for asset and note we'd have to choose different names and then I'm not sure how much benefit there is then.
So maybe re-exporting is the best strategy for asset and note?

For asset, I think we can just have a single asset module which contains a union of procedures that we need both in the kernel and in miden library - and we use this module in both. Or would this not work for some reason?

For note, we might still have to have the _util module as we have now (or at least I don't see a better solution).

PhilippGackstatter · 2025-01-15T10:00:50Z

For asset, I think we can just have a single asset module which contains a union of procedures that we need both in the kernel and in miden library - and we use this module in both. Or would this not work for some reason?

Ah I see what you mean. I initially understood it as only union of asset_utils and miden::asset. But sure, we can make a single asset.masm file with everything.

Edit: Ah no, miden-lib/asm/kernels/transaction/lib/asset.masm depends on kernel::account so we can't unionize that.

For note, we might still have to have the _util module as we have now (or at least I don't see a better solution).

Agreed.

PhilippGackstatter · 2025-01-15T13:20:16Z

I tried combining these files:

miden-lib/asm/kernels/transaction/lib/asset.masm
miden-lib/asm/miden/asset.masm
miden-lib/asm/utils/asset_utils.masm

into a single file miden-lib/asm/utils/asset.masm and then include it in the kernel as kernel::asset and in miden lib as miden::asset. The issue with this is that miden::asset::create_fungible_asset calls miden::account::get_id which is not a pure procedure. So not all asset procedures can be put into the same file, and as mentioned further above, we can't have multiple ones because their names would collide.

Even if this would not be a problem like for account_id, there's another issue: Let's assume we have a shared module account_id.masm and want to make this a module in the kernel or miden library. If we add account_id.masm as a miden::account_id module to the assembler for the assembly of MidenLib, then miden lib will assemble successfully, but anyone using the miden::account_id module and imports only MidenLib will get an undefined module 'miden::account_id' error. This is because we only added miden::account_id for assembly of MidenLib, but it does not get added to MidenLib itself.
From my understanding we either have to reexport them for them to be copied in, or we'd have to manually parse the different modules and then call Assembler:assemble_library on all of them to be assembled into a single library. The latter is a bit of work, but given the relatively simple directory structure of the kernel this could be done. But the latter would only be a solution for account_id, asset and note must still be reexported. At least that's my current understanding.

Please correct me if I'm wrong, but I think even with library vendoring we'd have the same problem. The only difference with vendoring would be that if utils was a Library, it's procedures would be copied in wherever they are used in the kernel or miden lib, so the utils library would not have to be provided to the processor at execution time. Vendoring is going to be useful for users assembling their account code with a library of their own, but it does not solve the problem for us of, for example, creating a single kernel library with an account_id module and all the other ones we have now.

So unless I'm missing something, I would probably (in another PR) explore this idea of manually combining multiple modules as mentioned above. What I don't yet know how to solve nicely is to create a single module out of multiple files, if somehow possible with the help of Assembler rathern than resorting to ugly hacks.

For now in this PR, I would only reexport procedures from kernel and miden lib from utils procedures and keep the public interface the same. Any thoughts?

PhilippGackstatter · 2025-01-16T16:44:52Z

Superseded by #1070.

Fumuran force-pushed the andrew-remove-duplicate-procedures branch 3 times, most recently from 787743f to b6267d9 Compare December 5, 2024 16:05

Fumuran requested a review from bobbinth December 5, 2024 16:08

bobbinth reviewed Dec 6, 2024

View reviewed changes

PhilippGackstatter mentioned this pull request Dec 11, 2024

Extend AccountId to two Felts and refactor creation process #982

Merged

26 tasks

Fumuran force-pushed the andrew-remove-duplicate-procedures branch from b6267d9 to 4454352 Compare December 18, 2024 23:42

Fumuran marked this pull request as ready for review December 19, 2024 13:05

Fumuran requested a review from bobbinth December 19, 2024 13:05

Fumuran added 3 commits December 20, 2024 03:55

feat: implement utils lib, remove duplicated procedures from miden an…

1c62f15

…d kernel libs

feat: utils module working state

7df9a00

refactor: optimize utils module path in masm

73fa206

Fumuran force-pushed the andrew-remove-duplicate-procedures branch from 4454352 to 73fa206 Compare December 20, 2024 01:04

Merge branch 'next' into andrew-remove-duplicate-procedures

d1e44eb

bobbinth reviewed Dec 27, 2024

View reviewed changes

PhilippGackstatter mentioned this pull request Jan 3, 2025

Account ID Follow-Up Tasks #1046

Closed

22 tasks

Fumuran added 4 commits January 9, 2025 18:45

Merge branch 'next' into andrew-remove-duplicate-procedures

0ae710a

refactor: move pure utils functions out of account

38e3fda

Merge branch 'next' into andrew-remove-duplicate-procedures

fec196c

refactor: move pure account procs to account_util

21f1671

Fumuran force-pushed the andrew-remove-duplicate-procedures branch from 67352f0 to 21f1671 Compare January 14, 2025 11:17

PhilippGackstatter added 3 commits January 14, 2025 13:56

feat: Move asset vault and asset util procs to utils

60d15ea

feat: Move pure procedures to note_utils

dcf53b9

feat: Add utils library to testing_assembler

fb441e0

Fumuran commented Jan 14, 2025

View reviewed changes

bobbinth reviewed Jan 15, 2025

View reviewed changes

chore: Move non-shared procedures back to kernel

1177f89

bobbinth mentioned this pull request Jan 16, 2025

feat: Rename objects to miden-objects and use crates/ dir #1065

Open

feat: Implement shared modules under different namespaces

ee3c662

PhilippGackstatter mentioned this pull request Jan 16, 2025

Move duplicate procedures to shared util modules #1070

Merged

PhilippGackstatter closed this Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove duplicate procedures from `miden-lib` #1002

Remove duplicate procedures from `miden-lib` #1002

Fumuran commented Dec 3, 2024 •

edited

Loading

Fumuran commented Dec 5, 2024

bobbinth left a comment

bobbinth Dec 6, 2024

bobbinth Dec 6, 2024

bobbinth Dec 6, 2024

bobbinth commented Dec 27, 2024

bobbinth Dec 27, 2024

Fumuran Jan 9, 2025

bobbinth Jan 10, 2025

Fumuran Jan 10, 2025

PhilippGackstatter commented Jan 14, 2025

Fumuran left a comment

Fumuran Jan 14, 2025

bobbinth left a comment

bobbinth Jan 15, 2025

bobbinth Jan 15, 2025

bobbinth Jan 15, 2025

bobbinth Jan 15, 2025

PhilippGackstatter commented Jan 15, 2025

bobbinth commented Jan 15, 2025

PhilippGackstatter commented Jan 15, 2025 •

edited

Loading

PhilippGackstatter commented Jan 15, 2025 •

edited

Loading

PhilippGackstatter commented Jan 16, 2025

		# HELPER PROCEDURES
		# =================================================================================================

		let utils_namespace = LibraryNamespace::new("utils").expect("invalid namespace");
		let utils_path = Path::new(ASM_DIR).join(UTILS_DIR);

		/// This assembler also includes the `utils` namespace which contains the modules defined
		/// in the utils directory (only under feature `std`).

		#[derive(Clone)]
		pub struct UtilsLib(Library);

Remove duplicate procedures from miden-lib #1002

Remove duplicate procedures from miden-lib #1002

Conversation

Fumuran commented Dec 3, 2024 • edited Loading

Fumuran commented Dec 5, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter commented Jan 14, 2025

Fumuran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter commented Jan 15, 2025

bobbinth commented Jan 15, 2025

PhilippGackstatter commented Jan 15, 2025 • edited Loading

PhilippGackstatter commented Jan 15, 2025 • edited Loading

PhilippGackstatter commented Jan 16, 2025

Remove duplicate procedures from `miden-lib` #1002

Remove duplicate procedures from `miden-lib` #1002

Fumuran commented Dec 3, 2024 •

edited

Loading

PhilippGackstatter commented Jan 15, 2025 •

edited

Loading

PhilippGackstatter commented Jan 15, 2025 •

edited

Loading